fix(openai): preserve .withResponse() on create() return value#19077
fix(openai): preserve .withResponse() on create() return value#19077naaa760 wants to merge 1 commit intogetsentry:developfrom
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| try { | ||
| result = originalMethod.apply(context, args) as typeof result; | ||
| } catch (error) { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { handled: false, type: 'auto.ai.openai', data: { function: methodPath } }, | ||
| }); | ||
| span.end(); | ||
| throw error; |
There was a problem hiding this comment.
Bug: A synchronous error in originalMethod causes a raw error to be thrown, instead of returning a rejected promise-like object with .withResponse(), breaking the API contract.
Severity: MEDIUM
Suggested Fix
To ensure consistent error handling, the instrumentMethod function should always return a promise-like object. Wrap the call to originalMethod in a Promise.resolve() or modify the structure to ensure that even synchronous errors result in a rejected promise that passes through wrapReturnValue. This maintains the API contract and allows callers to reliably handle all errors with promise-based mechanisms like .catch() and access .withResponse().
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/src/tracing/openai/index.ts#L291-L299
Potential issue: The `instrumentMethod` function uses `startSpanManual`, which re-throws
synchronous errors immediately. If the wrapped `originalMethod` (e.g., from the OpenAI
SDK) throws a synchronous error, the `try...catch` block within `startSpanManual`'s
callback will re-throw it. This prevents the `wrapReturnValue` function from being
called. As a result, the caller receives a raw `Error` object instead of a rejected
promise-like object that includes the `.withResponse()` method. This is a breaking
change from the previous behavior, which always returned a promise, and creates
inconsistent error handling between synchronous and asynchronous failures.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| return withResponseThenable.then((payload: { data: unknown; response: unknown }) => { | ||
| addResponseAttributes(span, payload.data, options.recordOutputs); | ||
| return payload; | ||
| }); |
There was a problem hiding this comment.
Non-streaming .withResponse() adds attributes after span ends
Medium Severity
When a user calls .withResponse() on a non-streaming request, both the chained promise handler (lines 201-206) and the .withResponse() handler (lines 249-252) process the response. The chained handler calls span.end(), but the .withResponse() handler then tries to call addResponseAttributes on the already-ended span. This happens because chained is created eagerly and its handlers run whenever the underlying promise resolves, regardless of whether the user uses .withResponse().
Additional Locations (1)
| }); | ||
| span.end(); | ||
| throw error; | ||
| }, |
There was a problem hiding this comment.
Non-streaming error handler missing span error status
Medium Severity
The non-streaming async error handler (lines 207-213) is missing a call to span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }). The streaming error handler (lines 192-199) and the synchronous error handler (lines 293-300) both properly set the span status on error, but this path was overlooked. This causes non-streaming request failures to not have their span marked as errored in Sentry traces.
| options: { recordOutputs?: boolean; recordInputs?: boolean }, | ||
| methodPath: InstrumentedMethod, | ||
| params: Record<string, unknown> | undefined, | ||
| operationName: string, |
There was a problem hiding this comment.
Unused params and operationName parameters in wrapReturnValue
Low Severity
The params and operationName parameters are declared in wrapReturnValue (lines 173-174) and passed from instrumentMethod (lines 305-307), but they are never used anywhere in the function body. This appears to be leftover dead code from refactoring.
|
Hey @naaa760 thanks for opening this PR! @RulaKhaled @nicohrubec, could you take a look at this please? |
|
Hey, thank you for opening the PR. I’ve already addressed this in #19122, so I’ll be closing this one. |
3 participants
PR description
fix: #19073